Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Send bound to streams. #471

Merged
merged 2 commits into from
Mar 6, 2022
Merged

Add Send bound to streams. #471

merged 2 commits into from
Mar 6, 2022

Conversation

sebpuetz
Copy link
Contributor

@sebpuetz sebpuetz commented Jan 19, 2022

I had to dig a bit into the different QueryStream implementations and the only reason for !Send I could find was the builder function passed to the builder in the ouroboros context. The self_referencing macro generates a signature on the current release of ouroboros that allows !Send constructors:

https://github.com/joshua-maros/ouroboros/blob/main/ouroboros_macro/src/info_structures.rs#L265

I forked ouroboros and added the Send bound on the constructor future and replaced the crates.io dependency with one pointing at my branch. After fixing up the Send requirements on the TransactionStream, a few other places in this crate needed + Send bounds.

This PR is not meant to be merged, it's mostly for documentation / demonstration of what would be required to get streams working in tokio::spawn context. If you're interested in getting this PR into better shape, we'll have to get into touch with the ouroboros maintainers to figure out if the missing Send bound is intentional.

I just recognized that TransactionStream::build can use a synchronous builder since the builder closure doesn't await any futures. That means the change on ouroboros is not even required to enable Send streams.

The first commit here is a solution that keeps the async constructor, the second one just removes the unused future on build

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, looks good! Thanks @sebpuetz

@billy1624 billy1624 requested a review from tyt2y3 January 20, 2022 02:48
@sebpuetz sebpuetz marked this pull request as ready for review January 20, 2022 08:00
@tyt2y3 tyt2y3 force-pushed the master branch 2 times, most recently from 7ce941b to 33a87d7 Compare February 6, 2022 06:39
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! it looks extremely useful!

@tyt2y3
Copy link
Member

tyt2y3 commented Feb 12, 2022

@billy1624 it seems to conflict with the changes you made to ConnectionTrait. can you help resolve?

@billy1624
Copy link
Member

Let me check

@billy1624
Copy link
Member

Hey @sebpuetz, please check and cherrypick billy1624@4bf35b4

@tyt2y3 tyt2y3 changed the base branch from master to pulls/471 March 6, 2022 14:10
@tyt2y3 tyt2y3 merged commit ccd0d97 into SeaQL:pulls/471 Mar 6, 2022
@tyt2y3 tyt2y3 mentioned this pull request Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants